-
Notifications
You must be signed in to change notification settings - Fork 564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
More backward-compatible changes to Trilinos 'develop' for updated TriBITS #10533
More backward-compatible changes to Trilinos 'develop' for updated TriBITS #10533
Conversation
I was getting an configure time error from CMake 3.19.1 with spaces before the math library in Trilinos_EXTRA_LINK_FLAGS with the sems-rhel7-clang-7.0.1-openmp-shared-release-debug build on by COE RHEL7 machine. Calling string(STRIP ...) takes care of the problem.
…/TriBITS#299) New TriBITS using modern CMake targets no longer sets the INCLUDE_DIRECTORIES directory property but instead passes along include dirs using INTERFACE_INCLUDE_DIRECTORIES target properties (like a good CMake project). This was a strange test that does not actually link to the Sacado library but yet needs a bunch of Sacado include dirs? We need to look into this and see how to make this work more cleanly either in Sacado or TriBITS because this is unfortunate.
…/TriBITS#464) Before, this was passing in: FAIL_REGULAR_EXPRESSION "FAIL;BUMMER" which just happened to be added as a list to the test property with the old TriBITS. But with the new TriBITS in TriBITSPub/TriBITS#464 which more correctly deals with semi-colons and list arguments, this was being set as a single property value "FAIL\\;BUMMER" which did not match the output. Passing in the list with: FAIL_REGULAR_EXPRESSION FAIL BUMMER works both with old and new TriBITS and is more logical given how CMake/CTest is supposed to be used. (The old TriBITS documentation was incorrect.)
…BITS#464) Tt turns out that these tests really did not need to pass a list of values to FAIL_REGULAR_EXPRESSION. Each test was really only looking for a single value of 'FAILED' or 'BUMMER'. And you can argue that the tests are stronger and better by taking out the other value that is not supposed to be printed in that test case.
82e6408
to
1bcbefc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sacado changes look good to me.
…rilinos#10534) This test code has not been enabled or built in over 10 years due to a defect added in commit: 7aabd5d "Isorropia: Make EpetraExt a required dependency" Author: Brent Perschbacher <bmpersc@sandia.gov> Date: Tue Feb 21 13:37:59 2012 -0700 (10 years ago) M packages/isorropia/cmake/Dependencies.cmake But note that HAVE_EPETRA was getting set to true unconditionally due to a defect in how it was set as: SET(HAVE_EPETRAEXT ${PACKAGE_NAME}_ENABLE_EpetraExt) instead of as dereferencing the variable with: SET(HAVE_EPETRAEXT ${${PACKAGE_NAME}_ENABLE_EpetraExt}) So the value "Isorropia_ENABLE_EpetraExt" evaluates to true in CMake so EpetraExt support was always required, even if EpetraExt support in Isorropia was disabled! So I just fixed this to make it clear that this was **always** on by changing this to: SET(HAVE_EPETRAEXT ON) This test code was getting switched on again with the new TriBITS that sets Isorropia_ENABLE_EpetraExt to TRUE, even for required dependencies (see PR
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: intel-19.0.5
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: python-3
Jenkins Parameters
Build InformationTest Name: _cuda_10.1.243
Jenkins Parameters
Using Repos:
Pull Request Author: bartlettroscoe |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: intel-19.0.5
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: python-3
Jenkins Parameters
Build InformationTest Name: _cuda_10.1.243
Jenkins Parameters
Console Output (last 100 lines) : Trilinos_pullrequest_gcc_8.3.0 # 7628 (click to expand)
Console Output (last 100 lines) : Trilinos_pullrequest_gcc_7.2.0_serial # 5124 (click to expand)
Console Output (last 100 lines) : Trilinos_pullrequest_gcc_7.2.0_debug # 5642 (click to expand)
Console Output (last 100 lines) : intel-19.0.5 # 291 (click to expand)
Console Output (last 100 lines) : Trilinos_pullrequest_clang_10.0.0 # 5327 (click to expand)
Console Output (last 100 lines) : python-3 # 1888 (click to expand)
Console Output (last 100 lines) : _cuda_10.1.243 # 996 (click to expand)
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: intel-19.0.5
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: python-3
Jenkins Parameters
Build InformationTest Name: _cuda_10.1.243
Jenkins Parameters
Using Repos:
Pull Request Author: bartlettroscoe |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: intel-19.0.5
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: python-3
Jenkins Parameters
Build InformationTest Name: _cuda_10.1.243
Jenkins Parameters
Console Output (last 100 lines) : Trilinos_pullrequest_gcc_8.3.0 # 7631 (click to expand)
Console Output (last 100 lines) : Trilinos_pullrequest_gcc_7.2.0_serial # 5127 (click to expand)
Console Output (last 100 lines) : Trilinos_pullrequest_gcc_7.2.0_debug # 5645 (click to expand)
Console Output (last 100 lines) : intel-19.0.5 # 294 (click to expand)
Console Output (last 100 lines) : Trilinos_pullrequest_clang_10.0.0 # 5330 (click to expand)
Console Output (last 100 lines) : python-3 # 1891 (click to expand)
Console Output (last 100 lines) : _cuda_10.1.243 # 999 (click to expand)
|
FYI: This PR and other PRs that happen to enable ShyLU_Node will never pass the intel-19.0.5 build and be allowed to merge until #10549 is addressed. |
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: python-3
Jenkins Parameters
Build InformationTest Name: _cuda_10.1.243
Jenkins Parameters
Using Repos:
Pull Request Author: bartlettroscoe |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: python-3
Jenkins Parameters
Build InformationTest Name: _cuda_10.1.243
Jenkins Parameters
Console Output (last 100 lines) : Trilinos_pullrequest_gcc_8.3.0 # 7655 (click to expand)
Console Output (last 100 lines) : Trilinos_pullrequest_gcc_7.2.0_serial # 5151 (click to expand)
Console Output (last 100 lines) : Trilinos_pullrequest_gcc_7.2.0_debug # 5669 (click to expand)
Console Output (last 100 lines) : Trilinos_pullrequest_intel_17.0.1 # 12647 (click to expand)
Console Output (last 100 lines) : Trilinos_pullrequest_clang_10.0.0 # 5353 (click to expand)
Console Output (last 100 lines) : python-3 # 1914 (click to expand)
Console Output (last 100 lines) : _cuda_10.1.243 # 1022 (click to expand)
|
Status Flag 'Pull Request AutoTester' - Failure: Timed out waiting for job Trilinos_pullrequest_intel_17.0.1 to start: Total Wait = 603
|
Man this PR is hard to get the testing to pass. And the failures are completely unrelated to the changes in the PR. The last PR iteration failed for several reasons:
But the autotester seemed to suggest that some of the builds were halted:
I don't understand all of this from the CDash dashboard. I will run the PR build again. |
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: python-3
Jenkins Parameters
Build InformationTest Name: _cuda_10.1.243
Jenkins Parameters
Using Repos:
Pull Request Author: bartlettroscoe |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: python-3
Jenkins Parameters
Build InformationTest Name: _cuda_10.1.243
Jenkins Parameters
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
Hello @keitat, can you please approve this PR so it can be merged to the Trilinos 'develop' branch? It will simplify further testing with updated TriBITS against Trilinos 'develop' so we can get this updated version of TriBITS merged to Trilinos 'develop'. |
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ keitat ]! |
Status Flag 'Pull Request AutoTester' - Pull Request will be Automerged |
Merge on Pull Request# 10533: IS A SUCCESS - Pull Request successfully merged |
TriBITSPub#299) This was snapshotted from the Trilinos commit: commit 8c1028df724eb0096cd688b381285112bc6f914a Author: Roscoe A. Bartlett <rabartl@sandia.gov> Date: Wed May 18 15:08:17 2022 -0600 FindTPLNetcdf.cmake: Lower-case function names (TriBITSPub#274) soon to be merged to Trilinos 'develop' from Trilinos PR #10533. However, this is not an snapshot as I made the following changing before creating this commit: * Kept the spelling fix for 'separated' made in TriBITS 'master' branch from commit c716a74; Author: Greg Sjaardema <gsjaardema@gmail.com>; Date: Mon Aug 2 16:02:20 2021 -0600; "Spelling fixes" * I removed a few trailing spaces at the end of some lines Why? There really is no value in using a different version of FindTPLNetcdf.cmake in TriBITS that what is used in Trilinos. When testing Trilinos against updated versions of TriBITS, you really want the logic to match. In the case in testing with Trilinos (as part of TriBITSPub#299 and testing with trilinos/Trilinos#10533), the old version of FindTPLNetcdf.cmake before this change was not properly setting TPL_Netcdf_PARALLEL which was not allowing the enable of the test SEACASIoss_exodus_fpp_serialize. With this sync, we get the exact same tests.
TriBITSPub#299) This was snapshotted from the Trilinos commit: commit 8c1028df724eb0096cd688b381285112bc6f914a Author: Roscoe A. Bartlett <rabartl@sandia.gov> Date: Wed May 18 15:08:17 2022 -0600 FindTPLNetcdf.cmake: Lower-case function names (TriBITSPub#274) soon to be merged to Trilinos 'develop' from Trilinos PR #10533. However, this is not an snapshot as I made the following changing before creating this commit: * Kept the spelling fix for 'separated' made in TriBITS 'master' branch from commit c716a74; Author: Greg Sjaardema <gsjaardema@gmail.com>; Date: Mon Aug 2 16:02:20 2021 -0600; "Spelling fixes" * I removed a few trailing spaces at the end of some lines Why? There really is no value in using a different version of FindTPLNetcdf.cmake in TriBITS that what is used in Trilinos. When testing Trilinos against updated versions of TriBITS, you really want the logic to match. In the case in testing with Trilinos (as part of TriBITSPub#299 and testing with trilinos/Trilinos#10533), the old version of FindTPLNetcdf.cmake before this change was not properly setting TPL_Netcdf_PARALLEL which was not allowing the enable of the test SEACASIoss_exodus_fpp_serialize. With this sync, we get the exact same tests.
This PR contains a few changes to Trilinos 'develop' needed to work with updated TriBITS or updated CMake (even without TriBITS changes).
See the individual commits for more details which include the following changes:
add_exectuable()
code in Sacado to work with old and new TriBITSNow that as part of the SEMS PR clang-7.0.1 build below, I also did some configure timings for enabling all of the Trilinos packages and we are seeing a 25% overall speedup in configure+generate times for the new TriBITS vs. the old TriBITS. The results were:
The details are given below.
Details for configure timing for SEMS PR clang-7.0.1 build (click to expand)
This is for the repos versions:
What is interesting is that the reported configure times are:
I wonder if local timings would show this out?
First the reference build with old TriBITS:
Then the updated build with new TriBITS:
So this reduction in configure and especially generate times with the new TriBITS implementation are real! But the larger real reduction is in the generation times:
It makes sense that the new TriBITS implementation configure step would be faster but I don't know why the generate step would be so much faster.
That is a reduction of 25%. That is a pretty significant reduction in configure+generate times!
Testing
ATDM Trilinos clang 7.0.1 build
I developed these fixes and made these changes to get the Trilinos test suite to pass with the ATDM Trilinos build configuration
sems-rhel7-clang-7.0.1-openmp-shared-release-debug
which I modified to use updated TriBITS. The testing results were:sems-rhel7-clang-7.0.1-openmp-shared-release-debug testing details (click to expand)
.
Here are the repo state and versions on crf450:
The env and driver scripts on crf450 are:
Loading env and (re)building and testing:
SEMS PR clang-7.0.1 build
I (sort of) reproduced the SEMS-based clang-10.0.0 PR build locally on my RHEL7 machine 'crf450'. After getting the TriBITS copy of FindTPLNetcdf.cmake to match what is in Trilinos 'develop' (see TriBITSPub/TriBITS@97f8d6c), I was able to get all passing tests with both the old and new TriBITS (in TriBITSPub/TriBITS#479). I posted to CDash shown at:
which shows:
The build Linux-clang-10.0.0.ref used the existing TriBITS under Trilihnos/cmake/tribits while the build Linux-clang-10.0.0.tribits usesd the new TriBITS in PR TriBITSPub/TriBITS#479.
NOTE: These builds actually used clang-7.0.1 instead of clang-10.0.0 as I reported in #10540. But the updated TriBITS build passed compared to the reference build so it does not really matter for this purpose.
More details are given below.
Details for SEMS clang-10.0.0 (clang-7.0.1) builds (click to expand)
.
The repo versions for these tests were:
Doing the updated TriBITS bulid:
The reference build with the old TriBITS was identical except for setting